✨ OPRUN-3722: Consolidate configuration #1790
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
tmshort
changed the title
tmshort
force-pushed
the
consolidate-config
branch
from
1e7f144 to
023f662
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1790 +/- ##
==========================================
- Coverage 68.39% 68.34% -0.06%
==========================================
Files 63 63
Lines 5117 5117
==========================================
- Hits 3500 3497 -3
- Misses 1388 1390 +2
- Partials 229 230 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
tmshort
force-pushed
the
consolidate-config
branch
3 times, most recently
from
0c5081c to
1205f2d
Compare
tmshort
changed the title
tmshort
force-pushed
the
consolidate-config
branch
2 times, most recently
from
6092506 to
1aeadbb
Compare
|
@tmshort Looks good me me. Let me know when it is readyu to approve it. |
Just waiting for everything to pass... |
config/overlays/tilt-local-dev/operator-controller/patches/dev-deployment.yaml
Outdated
Show resolved
Hide resolved
This moves all the configuration into the root config directory. The config/README.md file shows the result. Updated Makefile, goreleaser, documentation and some of the unit test code. Kept CRDs in their original locations, but made copies to the new locations to keep the verify-crd-compatibility target working properly. Once this is merged, a followup will remove the CRDs from the original locations and update the verify-crd-compatibility target. It also tries to make catalogd less of a second-class piece of code. I compared the resulting operator-controller.yaml with main, and with the exception of `image` locations, they are identical. Signed-off-by: Todd Short <[email protected]>
tmshort
force-pushed
the
consolidate-config
branch
from
1aeadbb to
2278c8a
Compare
How did you do this comparison? I used |
I used This eliminates duplicates, since the second identically-named-typed resource overwrites the first one. If you include |
| @@ -0,0 +1,441 @@ | |||
| --- | |||
There was a problem hiding this comment.
Should the operator-controller CRD not be under base/operator-cotroller//crd/bases as well?
There was a problem hiding this comment.
I did move the ClusterExtension CRD, and it is under config/base/operator-controller/crd/bases:
https://github.com/operator-framework/operator-controller/pull/1790/files#diff-86654bcc3610d7644076892e7bc003a1af8040ecb7e4ac233883d0cf563c8561
|
Diff with main operator-controller.yaml: |
| name: controller-manager | ||
| namespace: system | ||
| name: operator-controller-controller-manager | ||
| namespace: olmv1-system |
There was a problem hiding this comment.
I think both was changed when we run kustomize build
Could we not leave as before for we centralize the namespace name and prefix in the kustomization.yaml instead?
There was a problem hiding this comment.
No, this is a patch for a specific deployment. Since the configurations are combined, there are two "controller-manager" deployments before their prefixes are applied. We need to be specific here - because this is a component "outside" the base resources.
There was a problem hiding this comment.
Error without these changes:
Error: accumulating components: accumulateDirectory: "recursed accumulation of path '/home/tshort/git/operator-framework/operator-controller/config/components/coverage': multiple matches for Id Deployment.v1.apps/controller-manager.system; failed to find unique target for patch Deployment.v1.apps/controller-manager.system"
There was a problem hiding this comment.
That is fine; we can always change things if we need them in the future as well.
i think we would need a kustomize for each
| name: controller-manager | ||
| namespace: system | ||
| name: operator-controller-controller-manager | ||
| namespace: olmv1-system |
There was a problem hiding this comment.
No, this is a patch for a specific deployment. Since the configurations are combined, there are two "controller-manager" deployments before their prefixes are applied. We need to be specific here - because this is a component "outside" the base resources.
There was a problem hiding this comment.
Error without these changes:
Error: accumulating components: accumulateDirectory: "recursed accumulation of path '/home/tshort/git/operator-framework/operator-controller/config/components/registries-conf': multiple matches for Id Deployment.v1.apps/controller-manager.system; failed to find unique target for patch Deployment.v1.apps/controller-manager.system"
| @@ -8,6 +8,3 @@ | |||
| - op: remove | |||
| # remove --leader-elect so container doesn't restart during breakpoints | |||
| path: /spec/template/spec/containers/0/args/2 | |||
| - op: add | |||
| path: /spec/template/spec/containers/0/args/- | |||
| value: --feature-gates=PreflightPermissions=true | |||
There was a problem hiding this comment.
Did we remove it intentionally?
There was a problem hiding this comment.
Yes, Joe asked for it to be removed.
#1790 (comment)
| apiVersion: kustomize.config.k8s.io/v1beta1 | ||
| kind: Kustomization | ||
| namespace: olmv1-system | ||
| namePrefix: operator-controller- |
There was a problem hiding this comment.
Could we keep the comments to clarify how it is used?
There was a problem hiding this comment.
I actually don't think the comments are useful here.
2a5e9b8
Fix #1341
This moves all the configuration into the root config directory. The config/README.md file shows the result.
Updated Makefile, goreleaser, documentation and some of the unit test code.
Kept CRDs in their original locations, but made copies to the new locations to keep the verify-crd-compatibility target working properly. Once this is merged, a followup will remove the CRDs from the original locations and update the verify-crd-compatibility target.
It also tries to make catalogd less of a second-class piece of code.
I compared the resulting operator-controller.yaml with main, and with the exception of
imagelocations, they are identical.Description
Reviewer Checklist